-
Notifications
You must be signed in to change notification settings - Fork 737
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use CreateTags instead of TagResources #129
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add how you test your changes? thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall changes look good. Left minor suggestions.
Could you please add some tests to validate the new changes?
package mock_ec2wrapper | ||
|
||
import ( | ||
reflect "reflect" | ||
|
||
"github.com/aws/aws-sdk-go/service/ec2" | ||
ec2 "github.com/aws/aws-sdk-go/service/ec2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this autogenerated using a newer version of mockgen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it, due to the slight differences. It was generated with go generate ./pkg/awsutils/...
pkg/awsutils/awsutils.go
Outdated
tags[eniTagKey] = aws.String(tagValue) | ||
log.Debugf("Trying to tag newly created eni: keys=%s, value=%s", eniTagKey, tagValue) | ||
for _, tag := range tags { | ||
log.Debugf("Trying to tag newly created eni: key=%s, value=%s", *tag.Key, *tag.Value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use aws.StringValue
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pkg/awsutils/awsutils.go
Outdated
awsAPIErrInc("TagResources", err) | ||
log.Warnf("Fail to tag the newly created eni %s %v", eniID, err) | ||
awsAPIErrInc("CreateTags", err) | ||
log.Warnf("Fail to tag the newly created eni %s: %v", eniID, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Fail/Failed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, done.
pkg/awsutils/awsutils.go
Outdated
Resource: "network-interface/" + eniID} | ||
arnString := eniARN.String() | ||
arns = append(arns, aws.String(arnString)) | ||
tags := []*ec2.Tag{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment with the sample tag keys and values for an ENI??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah.
pkg/awsutils/awsutils.go
Outdated
} else { | ||
log.Debugf("Tag the newly created eni with arn: %s", arnString) | ||
log.Debugf("Tag the newly created eni: %s", eniID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please rephrase this message to
"Successfully tagged eni: %s", eniID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, makes more sense.
* Also introduce two new tags: - node.k8s.amazonaws.com/instance_id - cluster.k8s.amazonaws.com/name (only tagged if the env var CLUSTER_NAME is present).
9d40633
to
c82dc6e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
Issue #, if available:
Fixes: #76
Description of changes:
Testing:
Created an EKS cluster with a node, which was created with the current CNI. I checked the tag on the ENI, it was the old format. I then applied CNI yaml with a container I built and scaled in and out the autoscaling group to get new nodes. The first time I did this with CLUSTER_NAME set:
Second time without:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.